Skip to content

Conversation

Yarwin
Copy link
Contributor

@Yarwin Yarwin commented Oct 4, 2025

What problem does this PR solve?

The obvious one – #1354, additionally I finally decided to address cfg leaking all over the codebase – i.e. handling docs-related cfg attr and features happens in docs module, not… everywhere else 😅.

I created another plugin registry for docs alone; initial logic by @Houtamelo directly edited plugin item with InherentImpl but I don't see it being maintainable (i.e. it is error prone + will devolve into chaos + mixing these boundaries is bad idea in general).

I'm not sure if additional registry is the best idea but I couldn't find anything better 🤷 and it is still gated behind register-docs feature.

I'm marking it as a feature, despite being an oversight 🧐.


Doc bugfixes and improvements

  • Bugfix: Handle docs in #[godot_api(secondary)]
  • Code tweaks: Separate docs modules to not pollute rest of the code with #[cfg(all(feature = "register-docs", since_api = "4.3"))]. Simplify registration logic.
  • Create separate PluginRegistry for Docs and Docs only.

@Yarwin Yarwin added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Oct 4, 2025
@Yarwin Yarwin linked an issue Oct 4, 2025 that may be closed by this pull request
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1355

@Yarwin Yarwin force-pushed the fix-docs-in-godot-api-secondary branch 2 times, most recently from b37c2c8 to 4830461 Compare October 4, 2025 09:33
@Yarwin Yarwin changed the title Class Docs – register docs in #[godot_api(secondary)], simplify registration logic Class Docs – register docs in #[godot_api(secondary)], simplify docs registration logic Oct 4, 2025
Houtamelo pushed a commit to Houtamelo/dead_gds_have_rights that referenced this pull request Oct 4, 2025
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

A big part of code has been deleted + re-added, Git won't detect as a move. I guess it's not easy to split this so that that's possible? (If not, that's fine, don't spend too much time).

Comment on lines 15 to 23
/// You should not manually construct this struct, but rather use [`DocsPlugin::new()`].
#[derive(Debug)]
pub struct DocsPlugin {
/// The name of the class to register docs for.
pub(crate) class_name: ClassId,

/// The actual item being registered.
pub item: DocsItem,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question on comment and field visibility: why is one pub(crate)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover after copy-pasting ClassPlugin (and extracting proper constructors).

pub enum DocsItem {
Struct(StructDocs),
InherentImpl(InherentImplDocs),
VirtualMethods(&'static str),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add a TraitImpl(TraitImplDocs) already, for consistency with the other enum variants? Also because we'd have a named field 🤔

* Copyright (c) godot-rust; Bromeon and contributors.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this to docs/mod.rs, I try not to use the other module style 🧐

/// XML attribute, as BBCode: `deprecated="DEPRECATED"`.
/// Contains whole paragraph annotated with a `@deprecated` tag.
deprecated_attr: String,
#[cfg(not(all(feature = "register-docs", since_api = "4.3")))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder if we can simplify those to just check register-docs, as we already verify (AFAIR?) the compatibility with api-* features?

Maybe in separate PR after this is merged, so there's not too many changes at once.

@Yarwin Yarwin force-pushed the fix-docs-in-godot-api-secondary branch from 4830461 to 9d302af Compare October 6, 2025 18:46
@Yarwin Yarwin marked this pull request as draft October 6, 2025 18:59
@Yarwin Yarwin force-pushed the fix-docs-in-godot-api-secondary branch 5 times, most recently from e34b423 to e1ed40d Compare October 6, 2025 19:19
@Yarwin Yarwin marked this pull request as ready for review October 6, 2025 19:20
Houtamelo and others added 2 commits October 6, 2025 21:21
- Bugfix: Handle docs in `#[godot_api(secondary)]`
- Code tweaks: Separate docs modules to not pollute rest of the code with `#[cfg(all(feature = "register-docs", since_api = "4.3"))]`. Simplify registration logic.
- Create separate PluginRegistry for Docs and Docs only.
@Yarwin Yarwin force-pushed the fix-docs-in-godot-api-secondary branch from e1ed40d to e1b84ba Compare October 6, 2025 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[godot_api(secondary)] inherent impl blocks do not register docs.
4 participants